Fix reference file logging to avoid logcat output - #12620
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
Review tier: Lite
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
 |
src/native/clr/host/os-bridge.cc — 💡 suggestion — The comment about “skipping logcat when logging to file” is now misleading because… |
What changed in this PR
This PR adjusts native CLR host reference logging to prevent GC bridge “gref/lref” traffic from flooding logcat when file-based reference logging is selected, while preserving existing file output behavior.
Changes:
- Gate
OSBridge::log_it()logcat writes so they occur only when no file is available (to == nullptr) or when logcat output was explicitly requested. - Keep file logging and stack-trace emission behavior intact for reference logging.
| File | Description |
|---|---|
| src/native/clr/host/os-bridge.cc | Avoid unconditional logcat writes when reference logging is routed to a file unless logcat is explicitly enabled. |
|
/review |
|
✅ Android PR Reviewer completed successfully! Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "azcliprod.blob.core.windows.net"See Network Configuration for more information.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Needs Changes
Findings: 0 errors · 0 warnings · 1 suggestion
The routing condition correctly preserves compact no-file logging, suppresses logcat for file-only reference logs, and retains explicit gref+/lref+ output. I left one inline suggestion for automated regression coverage.
CI build #1576487 is currently red: Windows > Build & Smoke Test failed while much of the matrix is still running. The Azure DevOps CLI was not authenticated in this environment, so I could not determine whether that failure is related; it needs to be triaged or cleared before merge.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
azcliprod.blob.core.windows.net
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "azcliprod.blob.core.windows.net"See Network Configuration for more information.
Generated by Android PR Reviewer for #12620 · gpt56 · 105.9 AIC · ⌖ 15.7 AIC · ⊞ 25.7K
Comment /review to run again
| void OSBridge::log_it (LogCategories category, std::string_view const& line, FILE *to, const char *const from, bool logcat_enabled) noexcept | ||
| { | ||
| log_writef (category, LogLevel::Info, "%.*s", static_cast<int>(line.length ()), line.data ()); | ||
| if (to == nullptr || logcat_enabled) { |
There was a problem hiding this comment.
🤖 💡 Testing — This predicate is now the contract separating compact (gref-/lref-), file-only, and file-plus-logcat modes, but the regression is covered only by manual validation. Please consider adding a focused test that exercises the routing combinations—especially to != nullptr && !logcat_enabled alongside the + case—so a future refactor cannot silently restore the logcat flood or suppress explicitly requested output.
Rule: Regression coverage
jonathanpeppers
left a comment
jonathanpeppers
left a comment
There was a problem hiding this comment.
Could the overall function be rewritten to be a bit clearer:
if (to == nullptr || logcat_enabled) {
log_writef (category, LogLevel::Info, "%.*s", static_cast<int>(line.length ()), line.data ());
}
// We skip logcat here when logging to file is enabled because _write_stack_trace will output to logcat as well, if enabled
if (to == nullptr) {
if (logcat_enabled) {
_write_stack_trace (nullptr, from, category);
}
return;
}I'm confused what is going on, now there are multiple if blocks checking the same values.
Description
OSBridge::log_it()unconditionally wrote reference log lines to logcat even whengref=<file>orlref=<file>selected file output without the+logcat option. Large GC-bridge diagnostics consequently flooded logcat and caused messages to be dropped.Only write the line to logcat when no file is available or logcat output was explicitly requested. File output remains unchanged. The shared CLR host source covers CoreCLR and NativeAOT.
Validation
src/native/native-clr.csprojfor all configured Android ABIs.src/native/native-nativeaot.csprojfor all configured Android ABIs.gc,gref=<file>: reference traffic was written to the file, while logcat contained only the file-open diagnostic. All 780 weak-reference creations and promotions were preserved in the file.